-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Move NaN tests to floats/mod.rs #143396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Move NaN tests to floats/mod.rs #143396
Conversation
f128: #[cfg(any(miri, target_has_reliable_f128_math))], | ||
}, | ||
test<Float> { | ||
use std::num::FpCategory as Fp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The as Fp
is copied from the original tests, but I don't actually see much reason for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move it to the top of this file instead? The short form makes a bit more sense with the other infinite/finite/etc tests that also use it. (Not that we need to shorten it but 🤷♂ )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually can't figure out a good way to do this.
https://github.com/rocurley/rust/pull/1/files shows my attempt, but it doesn't compile. To use the top-level import, I added a use super::*;
to the const module, but this breaks the shadowing done to switch between const and non-const assert_biteq
:
error[E0659]: `assert_biteq` is ambiguous
--> library/coretests/tests/floats/mod.rs:283:9
|
283 | assert_biteq!((0.0 as Float).min(0.0), 0.0);
| ^^^^^^^^^^^^ ambiguous name
|
= note: ambiguous because of a conflict between a name from a glob import and a macro-expanded name in the same module during import or macro resolution
note: `assert_biteq` could refer to the macro imported here
--> library/coretests/tests/floats/mod.rs:196:21
|
140 | / macro_rules! float_test {
141 | | (
142 | | name: $name:ident,
143 | | attrs: {
... |
196 | | assert_biteq_const as assert_biteq,
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
... |
228 | | };
229 | | }
| |_- in this expansion of `float_test!`
...
276 | / float_test! {
277 | | name: min,
278 | | attrs: {
279 | | f16: #[cfg(any(miri, target_has_reliable_f16_math))],
... |
304 | | }
| |_- in this macro invocation
note: `assert_biteq` could also refer to the macro imported here
--> library/coretests/tests/floats/mod.rs:191:21
|
140 | / macro_rules! float_test {
141 | | (
142 | | name: $name:ident,
143 | | attrs: {
... |
191 | | use super::*;
| | ^^^^^^^^
... |
228 | | };
229 | | }
| |_- in this expansion of `float_test!`
...
276 | / float_test! {
277 | | name: min,
278 | | attrs: {
279 | | f16: #[cfg(any(miri, target_has_reliable_f16_math))],
... |
304 | | }
| |_- in this macro invocation
= help: consider adding an explicit import of `assert_biteq` to disambiguate
I could instead import it explicitly in the macro, but that doesn't seem great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(AFAICT this is only a problem because the "specific imports shadow glob imports" rule doesn't work in a macro? If I inline float_test
it works just fine.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/rocurley/rust/pull/1/files shows my attempt, but it doesn't compile. To use the top-level import, I added a
use super::*;
to the const module, but this breaks the shadowing done to switch between const and non-constassert_biteq
:
Could you change it to super::Fp
rather than super::*
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that doesn't work then don't bother trying too hard to fix it. Please just squash then r=me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that works. Done. It seemed a little awkward to put every import into the macro, but it's also a bit awkward to have to import everything per test.
r=tgross35
assert!(!nan.is_normal()); | ||
assert!(nan.is_sign_positive()); | ||
assert!(!nan.is_sign_negative()); | ||
assert!(matches!(nan.classify(), Fp::Nan)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is changed from an assert_eq
to work in a const context.
I just opened that issue to track this, but for review I think it'd be better to |
f16: #[cfg(any(miri, target_has_reliable_f16_math))], | ||
f128: #[cfg(any(miri, target_has_reliable_f128_math))], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, noticed something; could you use target_has_reliable_f{16,128}
rather than the _math
versions? _math
is only for anything that calls libm
functions, which these don't.
Mind also moving this above the other float_test
instances? Might as well keep things ordered setup-> basic tests -> math tests, like the other test files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(just updating the status)
@rustbot review |
950099d
to
90fdf16
Compare
90fdf16
to
bc2001f
Compare
…gross35 Move NaN tests to floats/mod.rs This PR moves NaN tests to `floats/mod.rs`, as discussed in rust-lang#141726. Since this is my first PR against Rust, I'm keeping it as small as possible, but I intend to work my way through the remaining tests and can do that work in this PR if that's preferable. r? RalfJung
This PR moves NaN tests to
floats/mod.rs
, as discussed in #141726. Since this is my first PR against Rust, I'm keeping it as small as possible, but I intend to work my way through the remaining tests and can do that work in this PR if that's preferable.r? RalfJung